feat: Add google.api.http support #1075

jas-chen · 2024-07-16T05:41:29Z

Fixes #948.

This PR adds support for generating google.api.http-based metadata for developers to implement an HTTP client based on it.

To retrieve the information, google-protobuf and related vendor codes are included. They will be loaded on demand to reduce the performance hit.

It also introduces a top-level CLI option http just like the nestJs option.

stephenh · 2024-07-17T13:14:02Z

Hi @jas-chen ! This looks amazing! Just scanning the PR, everything looks great, but I won't have time to review more thoroughly until this weekend, if that's okay...I'll take a closer look, and maybe leave a few minor comments/suggestions then. Thank you!

stephenh

Hi @jas-chen ; thanks again for the PR, I had a chance to leave some feedback.

I'd really like to avoid vendoring the other code generator, and re-deserializing the request--lmk if you have ideas for how to avoid that by getting ts-proto-descriptors to have the info you need/want.

Also, per the comment, would love to have this PR (or a follow up) come with mapped types / runtime support to "just make a client" for users to use out-of-the-box.

Thanks!

stephenh · 2024-07-20T20:15:17Z

HTTP.markdown

+## Client implementation example
+
+```typescript
+// This is just an example, do not use it directly.


Hey @jas-chen , I think this is a great so far--I'm wondering if we could further and remove the need for this createApi method...

I.e. if a user has ~10 services with ~10 methods each, they''ll have to call createApi against each of them--granted, they could write a method to do that in a loop...

Like our current non-google impl looks like:

export interface MyService { MyMethod(request: RequestType): Promise<ResponseType>; } export const MyServiceServiceName = "MyService"; export class MyServiceClientImpl implements MyService { private readonly rpc: Rpc; private readonly service: string; constructor(rpc: Rpc, opts?: { service?: string }) { this.service = opts?.service || MyServiceServiceName; this.rpc = rpc; this.MyMethod = this.MyMethod.bind(this); } MyMethod(request: RequestType): Promise<ResponseType> { const data = RequestType.encode(request).finish(); const promise = this.rpc.request(this.service, "MyMethod", data); return promise.then((data) => ResponseType.decode(_m0.Reader.create(data))); } }

And it seems like it'd be nice to provide a similar sort of API to users...

I suppose with the export const Messaging, the users could provide their own mapped types & helper methods to achieve this same output, i.e. something like:

// creates the `MessagingService.MyMethod` / `MessagingService.updateMessage` interface type MessagingService = ClientService<typeof Messaging>; const client: MessagingService = createClient(Messaging); await client.getMessage(...);

I'm thinking it'd be great for ts-proto to eventually provide those ClientService and createClient implementations (i.e. that could automatically parse & replace the {message_id} based on the rules in their docs).

Granted, sometimes I've just codegen-d out the export interface MyService { ... } and export class MyServiceImpl, which was a very old-school approach code generation, but it's definitely doable to use TS mapped types & runtime libraries to achieve the same effect.

Wdyt about the goal of shipping a ClientService & createClient method that took in the Messaging config? Maybe it could be like a ts-proto-google-http runtime library, which didn't bloat the codegen output. Would you be interesting in contributing that as well as this config output?

Thanks!

I updated the client implementation example code, the createApi takes a service definition now.

README.markdown

package.json

src/context.ts

jas-chen · 2024-07-22T07:29:15Z

Hi @stephenh,

Thank you for reviewing my PR. I want to confirm this point first:

I'd really like to avoid vendoring the other code generator, and re-deserializing the request--lmk if you have ideas for how to avoid that by getting ts-proto-descriptors to have the info you need/want.

I tried using ts-proto-descriptors to generate code from google/api/annotations.proto. However, I couldn't find a way to generate code that injects HttpRule http = 72295728; to google.protobuf.MethodOptions.

https://github.com/googleapis/googleapis/blob/556515b4a1a60b38e8a23d7ac0171c9cd3a87c7a/google/api/annotations.proto#L28-L31

To be more specific, I want to make protos/google/protobuf/descriptor.ts to have additional code

// Add to the top of the file
const annotations = require('../api/annotations');
const http = require('../api/http');

// Add one more case in MethodOptions.decode
case annotations.http.number:
  if (tag !== annotations.http.tag) {
      break;
  }
  message.http = http.HttpRule.decode(reader, reader.uint32());
  continue;

Do you have any suggestions?

stephenh · 2024-07-24T02:14:39Z

Hi @jas-chen !

I think we can do this like this:

#1078

Unfortunately it looks like we might have a bug in our extensions support, because I'm getting this error:

[I] ~/o/t/protos (add-api-protos|✔) $ ./build.sh
google/api/annotations.ts:19:21 - error TS2345: Argument of type 'HttpRule | undefined' is not assignable to parameter of type 'HttpRule'.
  Type 'undefined' is not assignable to type 'HttpRule'.

19     HttpRule.encode(value, writer.fork()).ldelim();
                       ~~~~~


Found 1 error in google/api/annotations.ts:19

On that branch...maybe we're just missing an undefined check in the generated code?

Do you have time to look into fixing ^? It probably involves updating the extension code in src/main.ts (#808 was the original PR that extension suport)...

If not, I can try and take a look soon. Thanks!

jas-chen · 2024-07-24T04:30:35Z

Hi @stephenh,

After much consideration of the API design, I decided to extend the current outputServices=generic-definitions feature to support google.api.http. This way, it won't introduce any API changes, and it makes sense to only generate definitions.

We can add a client implementation later if needed.

Regarding the google/api/annotations.proto issue, thank you for providing #1078 as a reference, but I don't see protos/google/protobuf/descriptor.ts changed. I added a workaround protos/extend-descriptor.cjs in this PR as a codemod to inject HTTP-related code into it.

Please help review it again and let me know what you think; thank you!

jas-chen · 2024-07-24T07:08:58Z

After testing this API with a bunch of .proto files I have, I realized that the generated code from grpc and google.api.http are not compatible because google.api.http requires onlyTypes=true while grpc can't work with that option. So we can't use the same option to handle them at once 😓 .

I pushed a commit 2715391 to introduce the outputServices=generic-google-api-http-definitions option.

DMXRoid · 2024-08-12T22:06:33Z

This is functionality that I need badly enough that I'm using @jas-chen 's fork :p Happy to chip in and fix bugs and such if that would be at all helpful towards getting this merged in.

jas-chen · 2024-08-21T02:12:15Z

@stephenh, are you reviewing this PR? Is there anything I need to change?

stephenh · 2024-08-21T13:16:44Z

@jas-chen hello! Sorry, I've just been really busy with work lately; I definitely want to review & accept this PR--I should be able to review/accept it this weekend when I have some time. Thanks again!

jas-chen · 2024-08-22T01:41:32Z

Hi @stephenh,
No worries at all—take your time. I was just checking in to see if there’s any update on the status of the PR. Thank you!

williamsk91 · 2024-10-03T05:03:04Z

@stephenh Hi 👋🏻
@jas-chen has moved company and I plan to take over his PR 🙏🏻

When you have the time can you please help check whether the direction of adding protos/google/* is a good idea or not?

Can I also ask when does protos/build.sh is supposed to be called? I looked through the codebase but couldn't find anything 🙇🏻

Also going forward, maybe it's better for me to open a new PR from my own fork referencing this PR? 🤔

angelorc · 2024-10-17T10:18:21Z

It seems that many of us need this PR :D
I tried the PR locally, unfortunately I am unable to generate google.http.api types

git clone https://github.com/jas-chen/ts-proto/
git checkout 948-http-support
yarn && yarn build

on the directory integration/google-api-http after the command

protoc --version # libprotoc 3.12.4

protoc \
  --plugin=../../protoc-gen-ts_proto \
  --ts_proto_out=./generated \
  --ts_proto_opt=onlyTypes=true,outputServices=generic-google-api-http-definitions \
  ./*.proto

I still get a simple.ts file without any google.http.api

export const protobufPackage = "";

export interface GetMessageRequest {
  messageId: string;
}

export interface GetMessageResponse {
  message: string;
}

export interface CreateMessageRequest {
  messageId: string;
  /** mapped to the body */
  message: string;
}

export interface CreateMessageResponse {
}

stephenh · 2024-10-28T14:21:22Z

Hey @williamsk91 , @angelorc , sorry, this is a larger PR in an area that I'm not very familiar (I don't personally use google.api.http), so reviewing this PR has been easy to fall to the bottom of the todo list. :-/ Sorry about that!

But, as long as you're patient with me reviewing it & landing it, yep, would definitely love to have this feature/PR ship in ts-proto.

Kinda looks like the PR needs rebased/spruced up, which is undertandable given it's probably just stale; lmk if you need anything from me, but otherwise thanks for tackling this!

jas-chen added 8 commits July 16, 2024 13:22

Add http support

68e8fbb

Format readme

709b03e

Update vendor generator

b0778af

Update readme

3a3ef40

Update package.json

b9d0a55

Refine HTTP implementation

b04e1a0

Fix Readme

54ef26c

Fix Readme

56693fe

stephenh reviewed Jul 20, 2024

View reviewed changes

Use ts-proto-descriptors

a1c3d5d

jas-chen added 2 commits July 24, 2024 13:18

Make generic-definitions support http

b1f61da

Remove HTTP.markdown

039d364

jas-chen changed the title ~~feat: Add http support~~ feat: Add google.api.http support Jul 24, 2024

Add generic-google-api-http-definitions

2715391

jas-chen requested a review from stephenh July 24, 2024 07:19

jas-chen added 8 commits July 24, 2024 16:20

Revert src/generate-generic-service-definition.ts

867034c

Update implementation example

fb30279

Update implementation example

09765ca

Fix implementation example

fc884a3

Simplify client implementation example

cada9a2

Fix client implementation

58dd958

Use google-gax

94bbb54

Fix body

bca889e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add google.api.http support #1075

feat: Add google.api.http support #1075

jas-chen commented Jul 16, 2024

stephenh commented Jul 17, 2024

stephenh left a comment

stephenh Jul 20, 2024

jas-chen Jul 24, 2024

jas-chen commented Jul 22, 2024 •

edited

Loading

stephenh commented Jul 24, 2024

jas-chen commented Jul 24, 2024 •

edited

Loading

jas-chen commented Jul 24, 2024

DMXRoid commented Aug 12, 2024

jas-chen commented Aug 21, 2024

stephenh commented Aug 21, 2024

jas-chen commented Aug 22, 2024

williamsk91 commented Oct 3, 2024

angelorc commented Oct 17, 2024

stephenh commented Oct 28, 2024

feat: Add google.api.http support #1075

Are you sure you want to change the base?

feat: Add google.api.http support #1075

Conversation

jas-chen commented Jul 16, 2024

stephenh commented Jul 17, 2024

stephenh left a comment

Choose a reason for hiding this comment

stephenh Jul 20, 2024

Choose a reason for hiding this comment

jas-chen Jul 24, 2024

Choose a reason for hiding this comment

jas-chen commented Jul 22, 2024 • edited Loading

stephenh commented Jul 24, 2024

jas-chen commented Jul 24, 2024 • edited Loading

jas-chen commented Jul 24, 2024

DMXRoid commented Aug 12, 2024

jas-chen commented Aug 21, 2024

stephenh commented Aug 21, 2024

jas-chen commented Aug 22, 2024

williamsk91 commented Oct 3, 2024

angelorc commented Oct 17, 2024

stephenh commented Oct 28, 2024

jas-chen commented Jul 22, 2024 •

edited

Loading

jas-chen commented Jul 24, 2024 •

edited

Loading